Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conversion from AbstractArray to mxarray #229

Merged

Conversation

tqml
Copy link
Collaborator

@tqml tqml commented Oct 24, 2024

Adds a conversion method to convert Julia abstract array to mxarray to avoid having them treated as structs in MATLAB.

Example:

a = transpose(rand(10)) # transpose
x = mxarray(a) # x is an actual matrix in matlab, was a struct before
y = jvalue(x) # the return value is now a matrix instead of a dictionary

Related issue #196

@tqml
Copy link
Collaborator Author

tqml commented Oct 25, 2024

I removed the definitions for Ranges and BitArrays since they are only be a special case of AbstractArray, would unnecessary allocate data and get copied to MATLAB anyway

Old defs:

mxarray(a::BitArray) = mxarray(convert(Array{Bool}, a))
mxarray(a::AbstractRange) = mxarray([a;])

New definition was tested with additional special matrix types such as Diagonal, transpose and LowerTriangular.

function mxarray(a::AbstractArray{T}) where {T<:MxRealNum}
    mx = mxarray(T, size(a))
    ptr = data_ptr(mx)
    na = length(a)
    dat = unsafe_wrap(Array{T}, ptr, na)
    for (i, ix) in enumerate(eachindex(a))
        dat[i] = a[ix]
    end
    return mx
end

@ViralBShah
Copy link
Contributor

@tqml Would it be helpful to give you commit access to be able to merge your PRs and perform maintenance on the package?

@tqml
Copy link
Collaborator Author

tqml commented Feb 6, 2025

Hey @ViralBShah
Are there other active maintainers as well? Having commit access to merge this PR would be fine, however I can not actively maintain it besides the bare minimum, because I do not really use this package anymore and my MATLAB student license will expire at some point (which will make testing difficult)

@ViralBShah
Copy link
Contributor

I don't know of other active maintainers. It is always welcome for someone to do the minimum to maintain a package, and make releases etc. It is not necessarily a commitment to take on major development work.

If you're ok to get this one merged and release, that would already be quite welcome.

@musm
Copy link
Collaborator

musm commented Feb 7, 2025

Yeah my apologies. I'm the last active maintainer of this package. Haven't been keeping up to speed with all the pull requests.

@musm
Copy link
Collaborator

musm commented Feb 7, 2025

In any case this looks good to me

@musm musm merged commit 8183663 into JuliaInterop:master Feb 7, 2025
3 checks passed
@@ -276,8 +277,30 @@ function mxarray(a::Array{T}) where T<:MxComplexNum
mx
end

mxarray(a::BitArray) = mxarray(convert(Array{Bool}, a))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the function below still work?

@ViralBShah
Copy link
Contributor

Thanks @musm.

@tqml
Copy link
Collaborator Author

tqml commented Feb 7, 2025

Thanks @ViralBShah for the invite and thanks @musm for merging this PR :)

@tqml tqml deleted the feature/conversion-for-abstractarray-type branch February 7, 2025 11:00
@ViralBShah
Copy link
Contributor

Is it worth tagging a new release? Seems like a few things have accumulated since the last release.

@tqml
Copy link
Collaborator Author

tqml commented Feb 9, 2025

Good idea, the last release also was also some time ago. Can we justify v0.9?
The different handling of matrix types is kinda a breaking change, but i can hardly imagine that people really use matlab structs for their non-standard matrices.

I'm also not familiar with the Julia release process. Do you just create a new Git Tag and some automation does the rest?

@tqml
Copy link
Collaborator Author

tqml commented Feb 9, 2025

If its fine for you, I would include #231 in the release as well, it adds some new conversations for tuples

@ViralBShah
Copy link
Contributor

ViralBShah commented Feb 9, 2025

Yes, please do include. The process of releasing is fairly straightforward. The way I do it is:

  1. Bump the version number in Project.toml in line with the semver spec. I think 0.9 is justified.
  2. Click the Register button on https://juliahub.com/ui/Packages (and login with your github account).

@tqml
Copy link
Collaborator Author

tqml commented Feb 10, 2025

@ViralBShah @musm are you familiar why the package needs a build.jl, hardcodes the matlab paths into a deps.jl file and subsequently stores Refs and Ptrs to functions that could be a ccall?

Were there issues to get the file path at "runtime" (or when imported)?
Or was this a limitation of earlier versions of Julia (considering this package has been around a while)

@ViralBShah
Copy link
Contributor

ViralBShah commented Feb 10, 2025

This definitely sounds like how things were done a few years ago. Much of this went away with BinaryBuilder and artifacts but a lot of that won’t help here. Still I wouldn’t be surprised if folks can do better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants